Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY #513

shouze · 2017-09-08T18:04:53Z

- What I did

- How I did it

By resetting uid/gid to 0/0 in the build context. Prior to that, made possible by adding chownOpts in pkg/archive tarWithOption moby/moby#34590

- How to verify it

go test -v ./cli/command/image -run TestRunBuildResetsUidAndGidInContext

- Description for the changelog

Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY

- A picture of a cute animal (not mandatory but encouraged)

codecov-io · 2017-09-08T18:09:29Z

Codecov Report

Merging #513 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage   49.06%   49.06%   +<.01%     
==========================================
  Files         200      200              
  Lines       16407    16408       +1     
==========================================
+ Hits         8050     8051       +1     
  Misses       7938     7938              
  Partials      419      419

shouze · 2017-09-08T18:34:08Z

@tonistiigi I need to write the test but looks like not so trivial to me ATM as buildCtx is internal concept of runBuild(). Still need to read the code. I also want to address --stream, will I need to make changes inside buildkit?

tonistiigi · 2017-09-08T18:39:04Z

@dnephin may have some tips for the test. For --stream I need to fix moby/moby#34708 first.

dnephin · 2017-09-08T18:53:38Z

@tonistiigi this would be really easy to test if we merged #294 , it would be a unit test on createBuildContextFromLocalDir()

shouze · 2017-09-09T23:51:30Z

@tonistiigi @dnephin ok I've added the test, ready for a review.

@tonistiigi ok so about --stream maybe I can make a dedicated PR as it's still an experimental feature while this PR fixes a legacy issue. WDYT?

shouze · 2017-09-10T10:57:06Z

@tonistiigi I've started moby/moby#34708 in moby/moby#34797 but looks like it's probably just more than an update, I'll take a look as a fsutil upgrade seems required too.

dnephin

Thanks! The test is looking good, but I have a couple suggestions for making it more readable.

The gotestyourself/fs helpers are new, so they weren't used on the existing test. I'll have to go back and update it to use the new functions.

dnephin · 2017-09-11T17:09:29Z

cli/command/image/build_test.go

+func TestRunBuildResetsUidAndGidInContext(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Invalid test on Windows")
+	}


We have some helpers for this, please use https://godoc.org/github.com/gotestyourself/gotestyourself/skip

skip.IfCondition(t, runtime.GOOS == "windows", "uid and gid not relevant on windows")

dnephin · 2017-09-11T17:11:53Z

cli/command/image/build_test.go

+	}
+	dest, err := ioutil.TempDir("", "test-build-context-dest")
+	require.NoError(t, err)
+	defer os.RemoveAll(dest)


We have helpers for this: https://godoc.org/github.com/gotestyourself/gotestyourself/fs

dest := fs.NewDir(t, "test-build-context-dest") defer dest.Remove() // use dest.Path() to get the path

dnephin · 2017-09-11T17:19:46Z

cli/command/image/build_test.go

+
+	ioutil.WriteFile(filepath.Join(dir, "foo"), []byte("some content"), 0644)
+	err = os.Lchown(filepath.Join(dir, "foo"), 65534, 65534)
+	require.NoError(t, err)


This test can use a Dockerfile from the context, instead of stdin. So dockerfile can be just the string, instead of a bytes.Buffer. and you don't need to set cli.SetIn(). Below you won't need the options.dockerfileName = "-" line either.

You can replace this block (line 45-53) with:

dir := fs.NewDir(t, "test-build-context", fs.WithFile("foo", "some content", AsUser(65534, 65534)), fs.WithFile("Dockerfile", dockerfile), ) defer dir.Remove()

You'll have to update vendor.conf to use gotestyourself v1.2.0 as WithFile() was fixed in that version to accept PathOp

shouze · 2017-09-12T10:05:18Z

@dnephin ready for a new review, thx for gotestyourself tips, it reduces a lot the noise when we read the test 👍

I can upgrade/modernize the other tests in another PR if you want.

dnephin

The test looks great, Thanks!

LGTM

dnephin · 2017-09-12T15:26:00Z

cli/command/image/build_test.go

+
+	fakeImageBuild := func(_ context.Context, context io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) {
+		buffer := new(bytes.Buffer)
+		tee := io.TeeReader(context, buffer)


Is this necessary? It doesn't seem like buffer is used.

Right, removed this unused intermediate buffer var 😉

dnephin · 2017-09-12T16:06:55Z

cli/command/image/build_test.go

+
+	fakeImageBuild := func(_ context.Context, context io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) {
+		tee := io.TeeReader(context, new(bytes.Buffer))
+		assert.NoError(t, archive.Untar(tee, dest.Path(), nil))


I think you can pass context here as the first argument to Untar() and remove tee altogether, no?

The reason I'm using io.TeeReader() in the other test is because I want to assert the header is as expected, but in this case the test isn't concerned with the compression, it just wants the files, so I don't think you need the extra buffer.

You're right, done!

tonistiigi · 2017-09-12T23:03:26Z

LGTM

@shouze Needs rebase

Signed-off-by: Sébastien HOUZÉ <[email protected]>

shouze · 2017-09-13T00:09:29Z

@tonistiigi thx, rebased!

shouze · 2017-09-13T18:55:58Z

@dnephin @tonistiigi I guess that after the merge a documentation upgrade would be required?

thaJeztah · 2017-09-14T09:19:21Z

/cc @mstanleyjones for documentation ^^

GordonTheTurtle added the status/0-triage label Sep 8, 2017

dnephin added status/2-code-review and removed status/0-triage labels Sep 8, 2017

shouze changed the title ~~Reset build ctx uid/gid to 0/0 on ADD/COPY~~ Reset uid and gid to 0 in the build context to avoid cache busting issues on ADD/COPY Sep 9, 2017

shouze changed the title ~~Reset uid and gid to 0 in the build context to avoid cache busting issues on ADD/COPY~~ Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY Sep 9, 2017

dnephin reviewed Sep 11, 2017

View reviewed changes

dnephin approved these changes Sep 12, 2017

View reviewed changes

dnephin reviewed Sep 12, 2017

View reviewed changes

Reset idPair during build to avoid cache busting

7e40761

Signed-off-by: Sébastien HOUZÉ <[email protected]>

dnephin merged commit 7b77ab5 into docker:master Sep 13, 2017

GordonTheTurtle added this to the 17.10.0 milestone Sep 13, 2017

dnephin added the docs/revisit label Sep 13, 2017

shouze deleted the reset-id-pair-during-build-to-avoid-cache-busting branch September 13, 2017 19:21

shouze mentioned this pull request Sep 14, 2017

Docker build cache does not ignore uid/gid on ADD or COPY moby/moby#32816

Closed

thaJeztah added the impact/changelog label Sep 14, 2017

shouze mentioned this pull request Sep 22, 2017

Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY in stream mode #549

Closed

nedix mentioned this pull request Dec 18, 2018

Multi-stage COPY --from should preserve ownership/permissions moby/moby#37123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY #513

Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY #513

shouze commented Sep 8, 2017 •

edited

Loading

codecov-io commented Sep 8, 2017 •

edited

Loading

shouze commented Sep 8, 2017

tonistiigi commented Sep 8, 2017

dnephin commented Sep 8, 2017

shouze commented Sep 9, 2017

shouze commented Sep 10, 2017

dnephin left a comment

dnephin Sep 11, 2017

dnephin Sep 11, 2017

dnephin Sep 11, 2017

shouze commented Sep 12, 2017 •

edited

Loading

dnephin left a comment

dnephin Sep 12, 2017

shouze Sep 12, 2017

dnephin Sep 12, 2017 •

edited

Loading

shouze Sep 12, 2017

tonistiigi commented Sep 12, 2017

shouze commented Sep 13, 2017

shouze commented Sep 13, 2017

thaJeztah commented Sep 14, 2017

Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY #513

Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY #513

Conversation

shouze commented Sep 8, 2017 • edited Loading

codecov-io commented Sep 8, 2017 • edited Loading

Codecov Report

shouze commented Sep 8, 2017

tonistiigi commented Sep 8, 2017

dnephin commented Sep 8, 2017

shouze commented Sep 9, 2017

shouze commented Sep 10, 2017

dnephin left a comment

Choose a reason for hiding this comment

dnephin Sep 11, 2017

Choose a reason for hiding this comment

dnephin Sep 11, 2017

Choose a reason for hiding this comment

dnephin Sep 11, 2017

Choose a reason for hiding this comment

shouze commented Sep 12, 2017 • edited Loading

dnephin left a comment

Choose a reason for hiding this comment

dnephin Sep 12, 2017

Choose a reason for hiding this comment

shouze Sep 12, 2017

Choose a reason for hiding this comment

dnephin Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

shouze Sep 12, 2017

Choose a reason for hiding this comment

tonistiigi commented Sep 12, 2017

shouze commented Sep 13, 2017

shouze commented Sep 13, 2017

thaJeztah commented Sep 14, 2017

shouze commented Sep 8, 2017 •

edited

Loading

codecov-io commented Sep 8, 2017 •

edited

Loading

shouze commented Sep 12, 2017 •

edited

Loading

dnephin Sep 12, 2017 •

edited

Loading